-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP-CS-Fixer の導入 #996
base: master
Are you sure you want to change the base?
PHP-CS-Fixer の導入 #996
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #996 +/- ##
==========================================
- Coverage 56.29% 56.23% -0.07%
==========================================
Files 75 75
Lines 8917 9046 +129
==========================================
+ Hits 5020 5087 +67
- Misses 3897 3959 +62
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
6f20c7d は、全て PHP-CS-Fixer による変更ですか? ローカルで bfc9f5d から実行を試したのですが、結果が異なる様子でして。
正しい実行手順を教えていただけますと幸いです。 |
@seasoftjapan
また、 |
テストが落ちているようなので、一旦 Draft に戻します |
未定義変数、連想配列の扱いに難がありそう |
- string_length_to_empty - ternary_to_elvis_operator
a796588
to
dee413e
Compare
@@ -741,12 +751,12 @@ public function isDelivFree($product_type_id) | |||
* @param SC_Customer $objCustomer ログイン中の SC_Customer インスタンス | |||
* @param integer $use_point 今回使用ポイント | |||
* @param integer|array $deliv_pref 配送先都道府県ID. | |||
複数に配送する場合は都道府県IDの配列 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
消失しているようです。
@@ -41,7 +40,7 @@ class API_CartAdd extends SC_Api_Abstract_Ex | |||
public function doAction($arrParam) | |||
{ | |||
$this->arrResponse = array( | |||
'Version' => ECCUBE_VERSION); | |||
'Version' => ECCUBE_VERSION, ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
カンマの後、改行してくれるオプションとか無いでしょうか?
いくつか、気持ちの悪い(中途半端な)印象の整形がありました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing_comma_in_multiline
が有効になっていると、複数行の配列で、末尾は改行しないパターンでこのようになってしまうようです。
複数行を強制するルールは実装されていないようなので、 trailing_comma_in_multiline => false
にしたいと思います
https://cs.symfony.com/doc/rules/control_structure/trailing_comma_in_multiline.html
@@ -125,27 +125,27 @@ protected function getProductsList($searchCondition, $disp_number, $startno, $li | |||
case 'price': | |||
$objProduct->setProductsOrder('price02', 'dtb_products_class', 'ASC'); | |||
break; | |||
// 販売価格が高い順 | |||
// 販売価格が高い順 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ、救えないですかね。case のコメントで、case の真上に欲しいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'statement_indentation' => ['stick_comment_to_next_continuous_control_statement' => true]
とすることで対応できそうです。
https://cs.symfony.com/doc/rules/whitespace/statement_indentation.html
が、 composer の依存関係の問題で、現在 php-cs-fixer 3.9.5 がインストールされています。
このバージョンでは上記オプションは未実装のため、利用できないようです。
PHPUnit のバージョンを上げることで、上記オプションも利用できるようになりそうなので、別途対応しようと思います。
.php-cs-fixer.dist.php
Outdated
'is_null' => false, | ||
'concat_space' => ['spacing' => 'one'], | ||
'native_constant_invocation' => false, | ||
'array_syntax' => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
デフォルト short だと思いますが、不都合ありそうでしたか?
個人的には、少なくとも混在するのは嫌で、統一するなら short 一択かなといった印象です。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変更が多くなりすぎるため、2.13からバージョンアップする際のコンフリクト解消が辛くなるんじゃないかと思って変換しないようにしてました。
- 2.13 の状態で、一旦 php-cs-fixer のルールを適用
- 2.17 のブランチをマージ
とすれば、少しはコンフリクトもマシになりそうですかね
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array_syntax のみの影響度合いは把握できていませんが、他の項目でスペースを変更されるのもマージ時には辛いかもとは思いました。私も対処方法として、2.13 側にも PHP-CS-Fixer を同じ設定で適用するのが良さそうに思いました。
.php-cs-fixer.dist.php
Outdated
'@Symfony:risky' => true, | ||
// '@PHP83Migration' => true, | ||
'psr_autoloading' => false, | ||
'single_quote' => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
デフォルトでなかなか良い感じの置換をする様子ですが、目に付くパターンありましたか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'single_quote'
ですかね。こちらも array_syntax
と同じ理由で false にしていました。
2.13 に php-cs-fixer を適用する手順で問題なさそうであれば、 true にしたいと思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'single_quote'
ですかね。
はい。1行を指定してコメントつけたつもりですが、上3行も自動引用されたようで、分かりにくくなり恐縮です。
.php-cs-fixer.dist.php
Outdated
'single_quote' => false, | ||
'is_null' => false, | ||
'concat_space' => ['spacing' => 'one'], | ||
'native_constant_invocation' => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これが適用されていなそうな置換が見受けられます。
例
- trigger_error('最大階層制限到達', E_USER_ERROR);
+ trigger_error('最大階層制限到達', \E_USER_ERROR);
当方環境 (PHP 8.3.9 / FROM ghcr.io/ec-cube/ec-cube2-php/8.3-apache) では、置換されないようです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
手元の環境の問題っぽいです。確認してみます
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
native_constant_invocation が適用されていたようですので対応します
throw new \LogicException(); | ||
} | ||
|
||
$rules = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
設定理由をソースコメントに残していただけると、参考になります。
特に無効化している項目ですが、具体的に不都合があったのか、何となく避けたかといった辺り分かると、今回は対応を見送っても、今後の対応拡大に繋げやすいと思います。
下に、何項目かコメントつけましたが、無理に今回対応しなくても、後々で良いのかなと思う部分もあります。
@@ -24,8 +24,8 @@ | |||
/** | |||
* バッチ処理用 の基底クラス. | |||
* | |||
* @package Page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Package は不要との判断でしょうか?
以前に、phpDocumentor で出力したときに「あー 分類されるんだ」と思ったくらいで、さほど必要になることは無いと思いますが。
rules 的には対象でない気がして、気になりました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Symfony
で phpdoc_no_package が適用されているようです。
https://cs.symfony.com/doc/rules/phpdoc/phpdoc_no_package.html
phpdoc_no_package => true
で良いと思います
@seasoftjapan ありがとうございます!おおむね対応可能だと思いますので、順次対応していきますね |
2.13.x のソースに対しても同様のルールを適用できそうなので、これを活用すれば旧バージョンからのマージや PHP8 対応も楽にできそう |
- コメント追記 - `single_quote => true` に変更(4系にあわせる) - `concat_space => ['spacing' => 'none']` に変更(4系にあわせる) - `array_syntax => true` に変更(4系にあわせる) - `phpdoc_scalar => true` に変更(phpdoc の int や bool を統一したい) - `data/smarty_extends` 及び `tests` にも適用
dee413e
to
872e0eb
Compare
872e0eb
to
5c6ecce
Compare
@seasoftjapan
各ルールにコメントも入れておきましたのでご確認お願いいたします🙇♂️ |
strlen($val)
で null チェックしているのを修正するのが主目的